-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: System Bars Plugin #8180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: System Bars Plugin #8180
Conversation
|
This looks great, what more is needed before merge? |
|
I am noticing that currently, without this plugin, the safe area and the status bar are working great on iOS -- it even changes the status bar text depending on light or dark mode when I toggle dark on the root: I am noticing this PR includes some styling on the status bar. Is there any way to have Android act like the iOS and have it "just work"? I also want to make sure this won't break what we have working fine on iOS. |
I am not sure if you have a correct understanding of this "inject css variables" thing. It doesn't inject |
|
I'd like to add that as of November 1st (actually earlier, but Google granted an extension to developers that depend on older SDKs) no one will be able to publish a release to the play store if they're not supporting edge-to-edge. So I think this should be given the utmost priority by the Capacitor team |
I just discovered all of this was only a problem with an Android Medium emulator. I just tried the Pixel 9 emulator and safe area is working great without any plugins. |
|
Hello everyone! Stay tuned, I'm on the verge of getting this past the finish line, The plan is to make some final changes and wrap this up this week. |
By "just work", you're expecting just the default status bar, without the fullscreen / transparent status bar style, correct? |
|
I'm curious which approach you've chosen to go with. I'm still advocating for going the native route. To demonstrate how that would work I made a simple demo proof of concept. Mind you that I've created this in like 5 minutes, so it's still not perfect, but you will get the main idea. Demo: https://github.com/tafelnl/capacitor-demo I also commented some todos which I haven't done because it's a proof of concept. |
As of right now on iOS, without this plugin, the status bar is transparent and switches the text color automatically to the right color when I set the root html class to dark mode and light mode. Not sure how this is working. |
android/capacitor/src/main/java/com/getcapacitor/plugin/SystemBars.java
Outdated
Show resolved
Hide resolved
|
Thanks @theproducer for this; this is much needed ! |
I think being able to control iOS's home indicator fits in to the use case here, thanks for the suggestion! |
|
Something i found out doing this plugin is hide and show doesn’t naturally update the css var for safe areas, so that something to take in account that would let us easy have reactive design. During my session i was not able to update the real value so i had to create new var. if you know how to handle this i would be glad ti be included too. |
|
Not sure how this PR relates to it, but you might want to take a look at this PR: ionic-team/capacitor-plugins#2388 |
|
I think it makes sense to remove the workaround logic from the keyboard plugin and incorporate it in the capacitor core/this PR. Otherwise all users would have to install the keyboard plugin in order to make it work correctly |
android/capacitor/src/main/java/com/getcapacitor/plugin/SystemBars.java
Outdated
Show resolved
Hide resolved
| Insets safeArea = insets.getInsets(WindowInsetsCompat.Type.systemBars() | WindowInsetsCompat.Type.displayCutout()); | ||
| injectSafeAreaCSS(safeArea.top, safeArea.right, safeArea.bottom, safeArea.left); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this case we should fallback to logic similar as to what adjustMarginsForEdgeToEdge does.
Also injecting the safe area insets as custom vars should be a toggleable option I think. And only respected if hasMetaViewportCover==true imo, so it stays consistent with the native inset behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this case we should fallback to logic similar as to what adjustMarginsForEdgeToEdge does.
You are referring to adding margins directly to the native view? That was meant to be a quick and dirty fix - we generally try to avoid manipulating the native views style-wise as much as possible to leave the power and final decision making in the hands of the web developer.
Using CSS env vars (albeit a bit convoluted because of the Android bug: var(--safe-area-inset-top, env(safe-area-inset-top, 0px));) would be the normal way of handling edge to edge behavior, and we want to stick as close to that as possible. Additionally, we have other requirements depending on these injected vars.
Also injecting the safe area insets as custom vars should be a toggleable option I think.
This is something we can do, I'll work on adding a config option for disabling the inset injections altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are referring to adding margins directly to the native view
Yes that's what I meant. It would align with iOS behavior. If a user does not enable edge-to-edge support through the viewport-fit=cover tag, the webview itself will already add margins for you. Otherwise every capacitor developer would need to add support for env variables. Which would be a major breaking change.
Ideally, of course, this is something that the Android team will just natively support out of the box. But iirc a few years/months ago one of the devs mentioned that it's not something they were looking into. I cannot find the comment anymore. I'll link it if I come across it
Co-authored-by: Tafel <[email protected]>
Actually, upon better looking into this, I think the workaround as is can be removed altogether. Instead we should do something like this inside the Insets systemBarsInsets = windowInsets.getInsets(WindowInsetsCompat.Type.systemBars() | WindowInsetsCompat.Type.displayCutout());
Insets imeInsets = windowInsets.getInsets(WindowInsetsCompat.Type.ime());
boolean keyboardVisible = windowInsets.isVisible(WindowInsetsCompat.Type.ime());
ViewGroup.MarginLayoutParams mlp = (ViewGroup.MarginLayoutParams) parent.getLayoutParams();
if (keyboardVisible) {
// https://issues.chromium.org/issues/457682720
// this is a workaround to push the webview behind the keyboard for webview versions that have a bug
// that causes the bottom inset to be incorrect if the IME is visible
boolean hasWebViewWithKeyboardBug = true; // chromium version with this bugfix is yet to be released, so the version is unknown still
if (hasWebViewWithKeyboardBug) {
mlp.bottomMargin = imeInsets.bottom - systemBarsInsets.bottom;
} else {
mlp.bottomMargin = imeInsets.bottom;
}
} else {
mlp.bottomMargin = 0;
}
parent.setLayoutParams(mlp);
return windowInsets;
I think this is much cleaner than the current workaround. Additionally it makes the workaround also work across config changes such as rotating the screen (the current workaround would break, which is a bug currently) |
OS-pedrogustavobilro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't fully finished reviewing, but leaving some comments questions, for now mostly minor stuff / documentation
| /** | ||
| * The style is based on the device appearance or the underlying content. | ||
| * If the device is using Dark mode, the statusbar text will be light. | ||
| * If the device is using Light mode, the statusbar text will be dark. | ||
| * | ||
| * @since 8.0.0 | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: technically it's not just the text right?
| /** | |
| * The style is based on the device appearance or the underlying content. | |
| * If the device is using Dark mode, the statusbar text will be light. | |
| * If the device is using Light mode, the statusbar text will be dark. | |
| * | |
| * @since 8.0.0 | |
| */ | |
| /** | |
| * The style is based on the device appearance or the underlying content. | |
| * If the device is using Dark mode, the system bars content will be light. | |
| * If the device is using Light mode, the system bars will be dark. | |
| * | |
| * @since 8.0.0 | |
| */ |
| * Style of the text and icons of the system bars. | ||
| * | ||
| * @since 8.0.0 | ||
| * @default default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be?
| * @default default | |
| * @default 'DEFAULT' |
| */ | ||
| SystemBars?: { | ||
| /** | ||
| * The style of the text and icons of the system bars. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing what would be the default value? 'DEFAULT'?
| style?: string; | ||
|
|
||
| /** | ||
| * Hide the system bars on start. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing what would be the default value? "false"?
| @@ -0,0 +1,273 @@ | |||
| # SystemBars | |||
|
|
|||
| The SystemBars API provides methods for configuring the style and visibility of the device System Bars / Status Bar. This API differs from the [Status Bar](https://capacitorjs.com/docs/apis/status-bar) plugin in that it is only intended to support modern edge to edge use cases moving forward. For legacy functionality, use the [Status Bar](https://capacitorjs.com/docs/apis/status-bar) plugin. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could use more information on how this core plugin works with status-bar, and when to use one or the other or both.
Unsure if this should reside here, in status bar documentation, or somewhere, just wanted to bring it up.
| setAnimation(options: SystemBarsAnimationOptions): Promise<void>; | ||
| } | ||
|
|
||
| export class SystemBarsPluginWeb extends WebPlugin implements SystemBarsPlugin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong but this core plugin will become the first (or at least the only one for Capacitor currently) that is only available in native mobile apps, not in web.
While for a lot of people this not being available in web would be obvious, maybe for some it wouldn't be. Wondering if we should make that explicit somewhere in the docs.
| if let style = getConfig().getString("style") { | ||
| setStyle(style: style) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: don't think this matters much, but on Android we're using the 'DEFAULT' value to explicitly set the style. Unsure if it matters much, since it should load with a default style, just wanted to mention this since it seems like a minor inconsistency.
| document.documentElement.style.setProperty("--safe-area-inset-right", "%dpx"); | ||
| document.documentElement.style.setProperty("--safe-area-inset-bottom", "%dpx"); | ||
| document.documentElement.style.setProperty("--safe-area-inset-left", "%dpx"); | ||
| window.dispatchEvent(new CustomEvent('safeAreaChanged')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this 'safeAreaChanged' event for? Something specific to Android WebView?
| CAPPluginMethod(name: "hide", returnType: CAPPluginReturnNone) | ||
| ] | ||
|
|
||
| public var hideHomeIndicator: Bool = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need this to be set outside of this class right? Or do we? If not, then
| public var hideHomeIndicator: Bool = false | |
| public private(set) var hideHomeIndicator: Bool = false |
| let inset = call.getString("inset")?.uppercased() | ||
|
|
||
| if let animation = call.getString("animation") { | ||
| setAnimation(animation: animation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: if we're uppercasing the style, we could do it for animation as well? Here in show, in loading config, and hide?
This PR introduces a new core plugin, System Bars, designed to be a modern take on the Status Bar plugin, managing Android System Bars, the iOS status bar, and fix issues related to broken safe area insets on Android.